Fix multi-world joint indexing in warp-to-mj coord conversion#2332
Fix multi-world joint indexing in warp-to-mj coord conversion#2332jsw7460 wants to merge 6 commits intonewton-physics:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFix per-world joint indexing in the MuJoCo kernel so joint metadata lookups use a global per-world joint index; add regression tests that verify replicated worlds with per-world body mass/COM yield consistent physics; record the fix in the changelog. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
newton/tests/test_multiworld_body_properties.py (1)
114-121: Remove unused variablebodies_per_worldin the reference loop.Line 117 computes
bodies_per_worldbut it's never used within the loop. For single-world models, direct indexing withbody_mass[0]is correct.♻️ Remove unused variable
for mass_val, label in [(mass_a, "A"), (mass_b, "B")]: model = self._build_articulated_model(num_worlds=1, device=device) - bodies_per_world = model.body_count // model.world_count body_mass = model.body_mass.numpy() body_mass[0] = mass_val # base body model.body_mass.assign(body_mass) ref_q[label] = self._run_sim(model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_multiworld_body_properties.py` around lines 114 - 121, Remove the unused local variable bodies_per_world from the reference loop in test_multiworld_body_properties.py: inside the loop that builds the model (see _build_articulated_model and use of model.body_mass), delete the computation bodies_per_world = model.body_count // model.world_count since it is never used; leave the rest of the loop (setting body_mass[0] and model.body_mass.assign(...), and calling self._run_sim(model) to populate ref_q) unchanged.newton/_src/solvers/mujoco/kernels.py (1)
578-580: Fix is correct; consider extracting the global index for consistency with the sibling kernel.The change correctly uses the global joint index for
joint_typeandjoint_childlookups, aligning with howjoint_q_startandjoint_qd_startare already indexed.For consistency with
convert_mj_coords_to_warp_kernel(line 453), consider precomputing the global index once:♻️ Optional: Extract global index to match sibling kernel pattern
worldid, jntid = wp.tid() + joint_id = joints_per_world * worldid + jntid + # Skip loop joints — they have no MuJoCo qpos/qvel entries q_i = mj_q_start[jntid] if q_i < 0: return qd_i = mj_qd_start[jntid] - type = joint_type[joints_per_world * worldid + jntid] - wq_i = joint_q_start[joints_per_world * worldid + jntid] - wqd_i = joint_qd_start[joints_per_world * worldid + jntid] + type = joint_type[joint_id] + wq_i = joint_q_start[joint_id] + wqd_i = joint_qd_start[joint_id]And at line 610:
# Get CoM offset in world frame - child = joint_child[joints_per_world * worldid + jntid] + child = joint_child[joint_id]Also applies to: 610-611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/mujoco/kernels.py` around lines 578 - 580, Extract the global joint index once (e.g., compute a local variable like global_jnt = joints_per_world * worldid + jntid) and use that variable for all array lookups instead of repeating the expression; update usages in the kernel where joint_type, joint_child, joint_q_start, and joint_qd_start are accessed (the occurrences around the current block and the similar lookups at the later spots around the 610-611 area) so indexing is consistent with convert_mj_coords_to_warp_kernel and avoids duplicated arithmetic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@newton/_src/solvers/mujoco/kernels.py`:
- Around line 578-580: Extract the global joint index once (e.g., compute a
local variable like global_jnt = joints_per_world * worldid + jntid) and use
that variable for all array lookups instead of repeating the expression; update
usages in the kernel where joint_type, joint_child, joint_q_start, and
joint_qd_start are accessed (the occurrences around the current block and the
similar lookups at the later spots around the 610-611 area) so indexing is
consistent with convert_mj_coords_to_warp_kernel and avoids duplicated
arithmetic.
In `@newton/tests/test_multiworld_body_properties.py`:
- Around line 114-121: Remove the unused local variable bodies_per_world from
the reference loop in test_multiworld_body_properties.py: inside the loop that
builds the model (see _build_articulated_model and use of model.body_mass),
delete the computation bodies_per_world = model.body_count // model.world_count
since it is never used; leave the rest of the loop (setting body_mass[0] and
model.body_mass.assign(...), and calling self._run_sim(model) to populate ref_q)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 47675d84-a083-42ea-bc7a-93c49c729551
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/tests/test_multiworld_body_properties.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 88: Edit the CHANGELOG.md entry so it’s a user-facing sentence (avoid
kernel/indexing internals) and move it to the end of the "Fixed" list under the
[Unreleased] section; replace the current line referencing
convert_warp_coords_to_mj_kernel and joint_type/joint_child with a concise
user-facing entry such as "Fix multi-world MuJoCo coordinate conversion using
the wrong body center of mass in replicated worlds" and append it as the last
item in the Fixed section (do not include implementation details like per-world
vs global indices or function symbols).
In `@newton/tests/test_multiworld_body_properties.py`:
- Line 1: The SPDX header in newton/tests/test_multiworld_body_properties.py
uses the wrong creation year (2025); update the SPDX copyright line by replacing
"2025" with "2026" so it reads the correct creation year (2026).
- Around line 135-145: The quaternion comparisons for free joints are
sign-ambiguous; implement a helper named _assert_joint_q_close (place it near
_run_sim()) that compares position parts ([:3]) and tail parts ([7:]) with
np.testing.assert_allclose, but for the quaternion slice [3:7] flips actual when
np.dot(actual_quat, expected_quat) < 0 before asserting; then replace the two
direct np.testing.assert_allclose calls that compare multi_q[0]/multi_q[1] to
ref_q["A"]/ref_q["B"] (and the similar block at lines 176-186) with calls to
_assert_joint_q_close(actual=multi_q[i], expected=ref_q[...], atol=1e-4,
err_msg=...) so q and -q are treated as equivalent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3a7c4321-241a-4663-b113-2980d40dd0ad
📒 Files selected for processing (2)
CHANGELOG.mdnewton/tests/test_multiworld_body_properties.py
|
🟡 Nice fix for the multi-world joint indexing in the coordinate conversion kernels. One more instance of the same per-world indexing pattern was missed in the forward kernel, and the new mass regression test doesn't fully exercise the code path it targets. 🟡 In axis_count = joint_dof_dim[jntid, 0] + joint_dof_dim[jntid, 1]The reverse kernel ( axis_count = joint_dof_dim[joint_id, 0] + joint_dof_dim[joint_id, 1]Today this is functionally benign because Suggested fix: axis_count = joint_dof_dim[joints_per_world * worldid + jntid, 0] + joint_dof_dim[joints_per_world * worldid + jntid, 1] |
867c6a0 to
8002c98
Compare
|
Thanks for the review! Both comments addressed:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/solvers/mujoco/kernels.py (2)
573-645:⚠️ Potential issue | 🟠 MajorApply the same global joint indexing in
convert_qfrc_actuator_from_mj_kernel.This hunk fixes the CoM lookup in
convert_warp_coords_to_mj_kernel, but the laterconvert_qfrc_actuator_from_mj_kernelstill indexes joint-indexed arrays with barejntid. Forworldid > 0, its FREE-joint branch will still use world 0'sjoint_child/body_comwhen shifting actuator torques, so per-world inertial-property updates remain wrong on that path.Suggested follow-up
def convert_qfrc_actuator_from_mj_kernel( ... ): worldid, jntid = wp.tid() + joint_id = joints_per_world * worldid + jntid # Skip loop joints — they have no MuJoCo DOF entries q_i = mj_q_start[jntid] if q_i < 0: return qd_i = mj_qd_start[jntid] wqd_i = joint_qd_start[joints_per_world * worldid + jntid] - type = joint_type[jntid] + jtype = joint_type[joint_id] - if type == JointType.FREE: + if jtype == JointType.FREE: ... - child = joint_child[jntid] + child = joint_child[joint_id] com_world = wp.quat_rotate(rot, body_com[child]) ... else: - axis_count = joint_dof_dim[jntid, 0] + joint_dof_dim[jntid, 1] + axis_count = joint_dof_dim[joint_id, 0] + joint_dof_dim[joint_id, 1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/mujoco/kernels.py` around lines 573 - 645, The convert_qfrc_actuator_from_mj_kernel uses bare jntid to index joint-indexed arrays (e.g., joint_child, body_com) causing wrong per-world lookups when worldid > 0; compute joint_id = joints_per_world * worldid + jntid (same as in convert_warp_coords_to_mj_kernel) and use joint_id for all accesses to joint_child, joint_type, joint_q_start, joint_qd_start, body_com, joint_dof_dim, etc., ensuring FREE-joint branches and actuator torque shifts reference the per-world joint entries rather than world-0 entries.
581-645:⚠️ Potential issue | 🟡 MinorRename
typetojtypeto avoid Ruff A001 (builtin shadowing).Line 581 shadows Python's builtin
type. Ruff rule A001 is enabled (via ruff.toml), andjtypeis the consistent naming convention used throughout the same file for similar variables.Minimal rename
- type = joint_type[joint_id] + jtype = joint_type[joint_id] ... - if type == JointType.FREE: + if jtype == JointType.FREE: ... - elif type == JointType.BALL: + elif jtype == JointType.BALL:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/mujoco/kernels.py` around lines 581 - 645, The local variable named type shadows Python builtin type and should be renamed to jtype: locate the assignment "type = joint_type[joint_id]" and replace it with "jtype = joint_type[joint_id]", then update all subsequent uses in this code block (e.g., the conditionals "type == JointType.FREE", "type == JointType.BALL", and any other references) to use jtype; ensure you also update any other occurrences of this same local variable within the same function so naming is consistent with the file's convention (use symbols joint_type, joint_id, joint_q_start, joint_q, joint_qd_start, joint_qd, JointType).
♻️ Duplicate comments (2)
newton/tests/test_multiworld_body_properties.py (2)
1-1:⚠️ Potential issue | 🟡 MinorUpdate the SPDX header to the 2026 creation year.
This file is new in the current PR, so the header should use
2026, not2025.As per coding guidelines, “In SPDX copyright lines, use the year the file was first created. Do not create date ranges or update the year when modifying a file.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_multiworld_body_properties.py` at line 1, Update the SPDX copyright header line in this new file by changing the creation year from 2025 to 2026: modify the SPDX line (the top-of-file comment starting with "SPDX-FileCopyrightText") to use "2026" and do not create a date range; ensure no other SPDX lines or ranges are added.
134-145:⚠️ Potential issue | 🟡 MinorMake the
joint_qcomparisons quaternion-sign invariant.These assertions still compare the free-joint quaternion verbatim.
qand-qencode the same pose, so this regression can fail even when the multi-world and single-world trajectories match. Please route them through a helper that flips one side when the quaternion dot product is negative.Based on learnings, “Quaternions in builder/test assertions use Warp’s XYZW convention (identity [0, 0, 0, 1]). In newton/tests/test_import_mjcf.py (e.g., test_fit_box_to_mesh_inertia_rotated), prefer identity/non-identity checks that account for q and -q equivalence (e.g., via np.abs on components).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_multiworld_body_properties.py` around lines 134 - 145, The quaternion comparisons between multi_q[...] and ref_q[...] need to be made sign-invariant: implement or call a helper (e.g., align_quaternion_sign(quat_ref, quat_test) or flip_quat_if_needed) that computes the dot product between the two 4D quaternions and multiplies one by -1 when the dot is negative, then use the aligned quaternions in the np.testing.assert_allclose calls for multi_q[0] vs ref_q["A"] and multi_q[1] vs ref_q["B"] so q and -q are treated as equivalent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@newton/_src/solvers/mujoco/kernels.py`:
- Around line 573-645: The convert_qfrc_actuator_from_mj_kernel uses bare jntid
to index joint-indexed arrays (e.g., joint_child, body_com) causing wrong
per-world lookups when worldid > 0; compute joint_id = joints_per_world *
worldid + jntid (same as in convert_warp_coords_to_mj_kernel) and use joint_id
for all accesses to joint_child, joint_type, joint_q_start, joint_qd_start,
body_com, joint_dof_dim, etc., ensuring FREE-joint branches and actuator torque
shifts reference the per-world joint entries rather than world-0 entries.
- Around line 581-645: The local variable named type shadows Python builtin type
and should be renamed to jtype: locate the assignment "type =
joint_type[joint_id]" and replace it with "jtype = joint_type[joint_id]", then
update all subsequent uses in this code block (e.g., the conditionals "type ==
JointType.FREE", "type == JointType.BALL", and any other references) to use
jtype; ensure you also update any other occurrences of this same local variable
within the same function so naming is consistent with the file's convention (use
symbols joint_type, joint_id, joint_q_start, joint_q, joint_qd_start, joint_qd,
JointType).
---
Duplicate comments:
In `@newton/tests/test_multiworld_body_properties.py`:
- Line 1: Update the SPDX copyright header line in this new file by changing the
creation year from 2025 to 2026: modify the SPDX line (the top-of-file comment
starting with "SPDX-FileCopyrightText") to use "2026" and do not create a date
range; ensure no other SPDX lines or ranges are added.
- Around line 134-145: The quaternion comparisons between multi_q[...] and
ref_q[...] need to be made sign-invariant: implement or call a helper (e.g.,
align_quaternion_sign(quat_ref, quat_test) or flip_quat_if_needed) that computes
the dot product between the two 4D quaternions and multiplies one by -1 when the
dot is negative, then use the aligned quaternions in the
np.testing.assert_allclose calls for multi_q[0] vs ref_q["A"] and multi_q[1] vs
ref_q["B"] so q and -q are treated as equivalent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1ecaeed4-5dd2-4ca8-be28-91f1834c6096
📒 Files selected for processing (3)
CHANGELOG.mdnewton/_src/solvers/mujoco/kernels.pynewton/tests/test_multiworld_body_properties.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Thanks for the fix — the core indexing change in kernels.py looks correct. A few things still need attention before this can land (the test device issue is the main CI blocker).
6a32136 to
7a572a3
Compare
convert_warp_coords_to_mj_kernel used per-world joint index for joint_type, joint_child, and joint_dof_dim lookups instead of the global index. This caused wrong body CoM references for worldid > 0. The sibling convert_mj_coords_to_warp_kernel already uses the global index. Extract joint_id = joints_per_world * worldid + jntid once and reuse it for all per-joint lookups. Rename the shadowed type local to jtype for consistency with other kernels in the file. Add regression test that exercises the CoM lookup path via per-world body_com overrides and non-zero initial angular velocity. The test uses get_test_devices() and auto-skips on CPU-only runners since the buggy code path only executes on CUDA (SolverMuJoCo with use_mujoco_cpu=False).
Reduce the test model to a single free-floating body per world (the articulated 2-link structure was not required to trigger the indexing bug). Build each world directly via add_builder with its target base CoM
7a572a3 to
66f2b01
Compare
adenzler-nvidia
left a comment
There was a problem hiding this comment.
All prior feedback addressed — multi-world indexing fix looks right, test covers the actual bug path (per-world CoM divergence). LGTM.
|
@jsw7460 needs another conflict resolve in the changelog, but looks very good now! let's get this in! |
Signed-off-by: SangwooShin <78995061+jsw7460@users.noreply.github.com>
Description
convert_warp_coords_to_mj_kernel used per-world joint index for joint_type and joint_child instead of the global index. This caused wrong body CoM lookups for worldid > 0. The sibling kernel convert_mj_coords_to_warp_kernel already uses the global index.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
test_perworld_com_produces_consistent_physicsFAILSBug fix
Steps to reproduce:
body_comper world viamodel.body_com+notify_model_changed(BODY_INERTIAL_PROPERTIES)convert_warp_coords_to_mj_kernelreads world 0'sjoint_childMinimal reproduction:
See
newton/tests/test_multiworld_body_properties.pySummary by CodeRabbit
Bug Fixes
Tests